Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor(api): Replaced OTP code from alphanumeric to numeric #230

Merged
merged 3 commits into from
May 21, 2024
Merged

refactor(api): Replaced OTP code from alphanumeric to numeric #230

merged 3 commits into from
May 21, 2024

Conversation

Ritika1705
Copy link
Contributor

@Ritika1705 Ritika1705 commented May 19, 2024 •
edited by codiumai-pr-agent-free bot
Loading

User description

Description

Replaced RandomUUid with Math.Random

Fixes #211

Dependencies

Mention any dependencies/packages used

Future Improvements

Mention any improvements to be done in future related to any file/feature

Mentions

Mention and tag the people

Screenshots of relevant screens

Add screenshots of relevant screens

Developer's checklist

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work

If changes are made in the code:

  • I have followed the coding guidelines
  • My changes in code generate no new warnings
  • My changes are breaking another fix/feature of the project
  • I have added test cases to show that my feature works
  • I have added relevant screenshots in my PR
  • There are no UI/UX issues

Documentation Update

  • This PR requires an update to the documentation at docs.keyshade.xyz
  • I have made the necessary updates to the documentation, or no documentation changes are required.

PR Type

enhancement

Description

  • Updated OTP generation method in auth.service.ts to generate numeric codes using Math.random() instead of alphanumeric codes with randomUUID().
  • This change affects both the creation and updating of OTP codes.

Changes walkthrough 📝

Relevant files
Enhancement
auth.service.ts
Change OTP Generation to Use Numeric Codes                             

apps/api/src/auth/service/auth.service.ts

  • Changed OTP generation from using randomUUID() to Math.random().
  • OTP codes are now purely numeric instead of alphanumeric.
    		•  +2/-2     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @Ritika1705 Ritika1705 requested a review from rajdip-b as a code owner May 19, 2024 08:19
    @codiumai-pr-agent-free CodiumAI PR-Agent (Free)
    Copy link
    Contributor

    PR Description updated to latest commit (2eefd19)

    @rajdip-b rajdip-b changed the title Replaced OTP code from alphanumeric to numeric refactor(api): Replaced OTP code from alphanumeric to numeric May 19, 2024
    @rajdip-b
    Copy link
    Member

    Hey @Ritika1705, thanks for making this PR! I will check this ASAP and try to merge it.

    @codiumai-pr-agent-free CodiumAI PR-Agent (Free)
    Copy link
    Contributor

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the PR involves a straightforward change in the OTP generation logic, replacing alphanumeric codes with numeric codes. The change is localized to a single file and method, making it relatively easy to review.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Predictability of OTP: The use of Math.random() for OTP generation may not provide sufficient randomness and security for OTP codes, which are critical for authentication purposes.

    🔒 Security concerns

    No

    @codiumai-pr-agent-free CodiumAI PR-Agent (Free)
    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Security
    Improve the security of OTP generation

    Replace Math.random().toString().substring(2, 8) with a more secure method of generating
    OTP codes. Using Math.random() for OTP can be predictable and insecure. Consider using a
    cryptographic library to generate a secure random number.

    apps/api/src/auth/service/auth.service.ts [48]

    -code: Math.random().toString().substring(2, 8),
+code: crypto.randomBytes(3).toString('hex'),
    Suggestion importance[1-10]: 10

    Why: The suggestion correctly identifies a significant security flaw in the use of Math.random() for OTP generation and proposes a more secure alternative using cryptographic methods.

    		10
    Maintainability
    Refactor OTP code generation to a separate method to reduce duplication

    Refactor the repeated code for generating the OTP code into a separate method. This will
    improve maintainability and reduce code duplication.

    apps/api/src/auth/service/auth.service.ts [48-52]

    -code: Math.random().toString().substring(2, 8),
+code: this.generateOtpCode(),
    Suggestion importance[1-10]: 8

    Why: The suggestion to refactor repeated code into a separate method is valid and would enhance maintainability and readability of the code.

    		8
    Possible bug
    Ensure consistent length for the OTP code

    Ensure that the generated OTP code has a consistent length. The current implementation
    using Math.random().toString().substring(2, 8) might not always return a string of length
    6, as the number could start with zeros.

    apps/api/src/auth/service/auth.service.ts [48]

    -code: Math.random().toString().substring(2, 8),
+code: Math.random().toString().slice(-6).padStart(6, '0'),
    Suggestion importance[1-10]: 7

    Why: This suggestion addresses a potential bug where the OTP code might not always be of consistent length, which is crucial for OTP functionality.

    		7
    Best practice
    Add error handling for OTP generation

    Consider adding error handling for the OTP generation process. This is important to ensure
    that the service can gracefully handle any failures during the OTP generation.

    apps/api/src/auth/service/auth.service.ts [48]

    -code: Math.random().toString().substring(2, 8),
+try {
+  code: Math.random().toString().substring(2, 8),
+} catch (error) {
+  // handle error appropriately
+}
    Suggestion importance[1-10]: 5

    Why: While adding error handling is generally a good practice, the specific method of OTP generation used (Math.random().toString().substring) does not typically throw errors that need to be caught, making this suggestion less relevant.

    		5

    @rajdip-b rajdip-b mentioned this pull request May 19, 2024
    Closed
    @kriptonian1
    Copy link
    Contributor

    Hey @Ritika1705 and @rajdip-b using Math.random is not the most secure way to generate opt you can read this amazing discussion in stackexchange crypto channel to know more on why link. I will rather recommend to use TOTP for that you can use otplib. Now, using math.random is not secure because it's a pseudo random number generator, which is not cryptographically secure.

    @rajdip-b
    Copy link
    Member

    @Ritika1705 hey! did you encounter any issue in the implementation? We are eager to merge your PR!

    @Ritika1705
    Copy link
    Contributor Author

    Ritika1705 commented May 20, 2024 via email

    @rajdip-b
    Copy link
    Member

    Oh! Alright no issues! Whenever you are free.

    @Ritika1705
    Copy link
    Contributor Author

    For better secure code generation, we can follow this article: https://medium.com/getpowerplay/a-one-time-password-otp-generator-npm-library-based-on-nanoid-79821a173798

    @rajdip-b
    Copy link
    Member

    Hey @Ritika1705, I went through some articles, and I feel that we can use the already existing crypto library. Maybe you can try out this suggestion?

    crypto.randomBytes(3).toString('hex'),

    I'm not sure how it will perform, but let's keep things simple - and use crypto package.

    @Ritika1705
    Copy link
    Contributor Author

    Ritika1705 commented May 20, 2024 •
    edited
    Loading

    Hey @Ritika1705 , I went through some articles, and I feel that we can use the already existing crypto library. Maybe you can try out this suggestion?

    crypto.randomBytes(3).toString('hex'),

    I'm not sure how it will perform, but let's keep things simple - and use crypto package.

    Hi @rajdip-b , I have upadated the code to use crypto package.

    The code BigInt(\0x${crypto.randomUUID().replace(/-/g, '')}).toString().substring(0, 6) generates a 6-character string by following these steps:

    • Generate a UUID: crypto.randomUUID() generates a version 4 UUID, which looks something like 550e8400-e29b-41d4-a716-446655440000.
    • Remove Hyphens: .replace(/-/g, '') removes all the hyphens from the UUID, resulting in a string like 550e8400e29b41d4a716446655440000.
    • Convert to BigInt: BigInt(\0x${uuid})converts this hexadecimal string to a BigInt. The0x prefix indicates that the string is in hexadecimal format.
    • Convert to String: .toString() converts the BigInt to its decimal string representation.
    • Extract 6 Digits: .substring(0, 6) takes the first 6 characters from the decimal string

    rajdip-b
    rajdip-b approved these changes May 21, 2024
    View reviewed changes
    Copy link
    Member

    @rajdip-b rajdip-b left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    LGTM!

    @sonarcloud SonarCloud
    Copy link

    sonarcloud bot commented May 21, 2024

    Quality Gate Passed Quality Gate passed

    Issues
    0 New issues
    0 Accepted issues

    Measures
    0 Security Hotspots
    No data about Coverage
    0.0% Duplication on New Code

    See analysis details on SonarCloud

    @rajdip-b rajdip-b merged commit f16162a into keyshade-xyz:develop May 21, 2024
    5 checks passed
    @rajdip-b rajdip-b mentioned this pull request May 21, 2024
    Merged
    10 tasks
    rajdip-b pushed a commit that referenced this pull request May 24, 2024
    @semantic-release-bot
    chore(release): 1.4.0 [skip ci]
    55862df 
    ## [1.4.0](v1.3.0...v1.4.0) (2024-05-24)

### 🚀 Features

* add example for health and email auth ([b834d25](b834d25))
* **api:** Add `minio-client` provider ([#237](#237)) ([cd71c5a](cd71c5a))
* **api:** Add feature to fork projects ([#239](#239)) ([3bab653](3bab653))
* **api:** Added feedback form module ([#210](#210)) ([ae1efd8](ae1efd8))
* **api:** Added Project Level Access  ([#221](#221)) ([564f5ed](564f5ed))
* **api:** Added support for changing email of users ([#233](#233)) ([5ea9a10](5ea9a10))
* implemented auth, ui for most, and fixed cors ([#217](#217)) ([feace86](feace86))
* **platfrom:** add delete method in api client ([#225](#225)) ([55cf09f](55cf09f))
* **postman:** add example for get_self and update_self ([e015acf](e015acf))
* **web:** Add and link privacy and tnc page ([#226](#226)) ([ec81eb9](ec81eb9))

### 🐛 Bug Fixes

* **web:** docker next config not found ([#228](#228)) ([afe3160](afe3160))

### 📚 Documentation

* Added docs regarding postman, and refactored architecture diagrams ([f1c9777](f1c9777))
* Fix typo in organization-of-code.md ([#234](#234)) ([11244a2](11244a2))

### 🔧 Miscellaneous Chores

* **api:** Get feedback forward email from process.env ([#236](#236)) ([204c9d1](204c9d1))
* **postman:** Initialized postman ([bb76384](bb76384))
* **release:** Update changelog config ([af91283](af91283))
* Remove swagger docs ([#220](#220)) ([7640299](7640299))

### 🔨 Code Refactoring

* **api:** Replaced OTP code from alphanumeric to numeric ([#230](#230)) ([f16162a](f16162a))
    @rajdip-b
    Copy link
    Member

    🎉 This PR is included in version 1.4.0 🎉

    The release is available on GitHub release

    Your semantic-release bot 📦🚀

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @rajdip-b rajdip-b rajdip-b approved these changes

    Assignees
    No one assigned
    Labels
    enhancement Review effort [1-5]: 2
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.

    API: Change OTP code from alphanumeric to numeric
    3 participants
    @Ritika1705 @rajdip-b @kriptonian1